Add Canvas Capture Strategy #4777
Conversation
…y-java into markushi/canvas-approach
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 3998a95 | 415.94 ms | 478.54 ms | 62.60 ms |
| 7314dbe | 437.83 ms | 505.64 ms | 67.81 ms |
| 3d205d0 | 352.15 ms | 432.53 ms | 80.38 ms |
| ee747ae | 554.98 ms | 611.50 ms | 56.52 ms |
| c8125f3 | 397.65 ms | 485.14 ms | 87.49 ms |
| 23d6b12 | 354.10 ms | 408.38 ms | 54.28 ms |
| ee747ae | 357.79 ms | 421.84 ms | 64.05 ms |
| d217708 | 411.22 ms | 430.86 ms | 19.63 ms |
| b3d8889 | 371.84 ms | 447.49 ms | 75.65 ms |
| d217708 | 375.27 ms | 415.68 ms | 40.41 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 3998a95 | 1.58 MiB | 2.10 MiB | 532.96 KiB |
| 7314dbe | 1.58 MiB | 2.10 MiB | 533.45 KiB |
| 3d205d0 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| c8125f3 | 1.58 MiB | 2.10 MiB | 532.32 KiB |
| 23d6b12 | 1.58 MiB | 2.10 MiB | 532.31 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| d217708 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| b3d8889 | 1.58 MiB | 2.10 MiB | 535.07 KiB |
| d217708 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
Previous results on branch: markushi/canvas-approach
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 0a0b1e3 | 353.33 ms | 370.31 ms | 16.98 ms |
| 8b7fc28 | 390.50 ms | 444.36 ms | 53.86 ms |
| 1b16487 | 375.34 ms | 440.81 ms | 65.47 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 0a0b1e3 | 1.58 MiB | 2.11 MiB | 542.83 KiB |
| 8b7fc28 | 1.58 MiB | 2.11 MiB | 543.01 KiB |
| 1b16487 | 1.58 MiB | 2.12 MiB | 547.06 KiB |
There was a problem hiding this comment.
are the changes in this class necessary? I don't see rootView or currentCaptureDelay being used anywhere
| private val processingHandler: Handler | ||
|
|
||
| init { | ||
| processingThread.start() |
There was a problem hiding this comment.
looking at this, we re-initialize ScreenshoRecorder every time there's a window/configuration change (which subsequently initializes this strategy), therefore we call thread.start and init the handler potentially multiple times during a single replay. Would it make sense to do that only once?
There was a problem hiding this comment.
Good point, I'll move it up a level so we can re-use the same instance
| return@Runnable | ||
| } | ||
| try { | ||
| holder.reader.setOnImageAvailableListener( |
There was a problem hiding this comment.
Is it possible to set this callback only once, or do we have to do it every time this Runnable runs?
| val hwImage = it?.acquireLatestImage() | ||
| if (hwImage != null) { | ||
| val hwScreenshot = toBitmap(hwImage) | ||
| screenshot = hwScreenshot |
There was a problem hiding this comment.
I guess we also have to synchronize access to screenshot (or use atomicRef) because it's being accessed from different threads potentially?
sentry-android-replay/src/main/java/io/sentry/android/replay/screenshot/CanvasStrategy.kt
Show resolved
Hide resolved
| } | ||
| if (holder == null) { | ||
| options.logger.log(SentryLevel.DEBUG, "No free Picture available, skipping capture") | ||
| executor.submitSafely(options, "screenshot_recorder.canvas", pictureRenderTask) |
There was a problem hiding this comment.
hm, is there a reason we submit here the task, even though we know there's no free picture? Or is this to submit any unprocessedPictures?
There was a problem hiding this comment.
I just did some refactoring which greatly simplifies this logic.
| } | ||
|
|
||
| private fun toBitmap(image: Image): Bitmap { | ||
| image.planes!!.let { |
There was a problem hiding this comment.
are we sure this is never null here 😅
| val color = paint.color | ||
| textPaint.color = Color.argb(100, Color.red(color), Color.green(color), Color.blue(color)) | ||
| tmpRect.offset(x.roundToInt(), y.roundToInt()) | ||
| drawRect(tmpRect, solidPaint) |
There was a problem hiding this comment.
oh btw, do you wanna use drawRoundRect so it aligns with the PixelCopy strategy? Same for images here
| } | ||
|
|
||
| // postAtFrontOfQueue to ensure the view hierarchy and bitmap are ase close in-sync as possible | ||
| mainLooperHandler.post { |
There was a problem hiding this comment.
I guess this slipped from the past, but we don't need to post anymore since we call capture() from the main looper now
3 participants
📜 Description
Adds an experimental option to enable an alternate way to capture screenshots, using a fake Canvas which draws rectangles instead of text, producing a safer way to mask all sensitive content.
💡 Motivation and Context
No masking issues.
💚 How did you test it?
Manually
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps